-
Notifications
You must be signed in to change notification settings - Fork 252
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[AY1920S1-CS2113-T14-3] WordUp #96
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General:
In terms of features and code I think y'all are on a good track :) What's lacking are unit tests (important!!) and header comments on public methods.
src/main/java/Dictionary/Word.java
Outdated
* String represents the closest time that user search | ||
* for a specific word | ||
*/ | ||
private String closetSearch; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
closestSearch you mean?
To clarify, closest time -> this is a string containing a time format?
} | ||
|
||
/** | ||
* Looks up for meaning of a specific word |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to add here that "if word is not in Wordbank, call OxfordCall to find the word"
import storage.Storage; | ||
import ui.Ui; | ||
|
||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add such headers for other command subclasses. Also, if the execute() is nontrivial (long code), do add a header explain what it does and how
return "https://od-api.oxforddictionaries.com/api/v2/entries/" + language + "/" + word_id + "?" + "fields=" + fields + "&strictMatch=" + strictMatch; | ||
} | ||
|
||
public static String doInBackground(String word) throws NoWordFoundException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add header comment for these public methods
src/main/java/parser/Parser.java
Outdated
throw new WrongDeleteFormatException(); | ||
} | ||
String[] wordAndTags = taskInfo[1].split("t/"); | ||
//delete word |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good, short comment that lets others zoom into specific code
src/main/java/parser/Parser.java
Outdated
if (taskInfo.length == 1) { | ||
throw new WrongAddFormatException(); | ||
} | ||
String[] wordDetail = taskInfo[1].split("w/"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again consider putting your "w/" etc as private static final attribs
src/main/java/storage/Storage.java
Outdated
} | ||
} | ||
|
||
public Stack<Word> loadHistoryFromFile() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Header comment. Make explicit the difference between loadHistory and loadWordBank (Stack structure, why)
Also, the code is very similar. Can both be done in one method, or are there times you have to load each separately?
} | ||
return wordBank; | ||
} catch (IOException e) { | ||
e.printStackTrace(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may want to feedback something more useful (to the user)
Updated UserGuide.adoc
Update instructions for running JAR file from terminal. Include Schedule and History feature instructions.
Add pictures for tags command in UG and search feature in DG
Add more test
Remove non-linked diagrams
@tran-quang-thanh @njw95 @chelsea148629 @kevin996231